feat: Improves logger to remove duplication#21
Conversation
Chocksy
force-pushed
the
feat/improved_logger
branch
from
89a5ed1 to
acbe735
Compare
|
Caution Review failedThe pull request is closed. WalkthroughThe changes enhance the logging system in the application. In Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Logger as Logger
participant LG as LogMessage
participant MG as MessageGroup
App->>Logger: Log(message, threshold)
Logger->>LG: Create LogMessage instance
Logger->>Logger: Determine if message has numeric value
alt Numeric value present
Logger->>MG: Add LogMessage to MessageGroup
else
Logger->>Logger: Store message in daily log dictionary
end
Logger->>Logger: Call _log_immediate for immediate processing
sequenceDiagram
participant CA as CentralAlgorithm
participant Logger as Logger
participant PS as PositionsStore
CA->>Logger: OnEndOfDay() invoked
Logger-->>CA: Debug log for symbol
CA->>Logger: process_and_output_daily_logs()
CA->>CA: OnEndOfAlgorithm() invoked
alt Live Mode
CA->>PS: store_positions()
else
CA->>Logger: process_and_output_daily_logs()
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (10)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
main.py (1)
118-123: Add docstring or comment clarifying log usage
These lines correctly add a debug message and callprocess_and_output_daily_logs(), ensuring daily logs are summarized. Consider adding a concise docstring or comment explaining how the logger handles daily vs. live mode scenarios to assist future maintainers.Tools/Logger.py (4)
6-9: Clean up unused imports
deque,OrderedDict,datetime,timedelta, andjsonare not used in this file. Removing them can reduce clutter.-from collections import deque, defaultdict, OrderedDict +from collections import defaultdict -from datetime import datetime, timedelta +from datetime import datetime -import json🧰 Tools
🪛 Ruff (0.8.2)
6-6:
collections.dequeimported but unusedRemove unused import
(F401)
6-6:
collections.OrderedDictimported but unusedRemove unused import
(F401)
7-7:
datetime.datetimeimported but unusedRemove unused import
(F401)
7-7:
datetime.timedeltaimported but unusedRemove unused import
(F401)
9-9:
jsonimported but unusedRemove unused import:
json(F401)
43-179: Robust grouping and analysis inMessageGroup
This design efficiently handles numeric analysis (trend, distribution) and textual grouping. It may be beneficial to handle extremely large message sets with caution, but for typical use cases, this is a clean approach.
193-224: Thoughtful logging logic
Implementing immediate logs for live mode vs. grouping for backtests ensures minimal overhead in real-time trading. As an enhancement, consider explicitly logging the reason why grouping is skipped in live mode for extra clarity.🧰 Tools
🪛 Ruff (0.8.2)
220-220: Do not use bare
except(E722)
281-297: Destructor usage
Callingprocess_and_output_daily_logs()in__del__helps in final cleanup. Be mindful that destructors might not always be called if references persist. Testing on the final run ensures no data is lost.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Tools/Logger.py(1 hunks)main.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
Tools/Logger.py
6-6: collections.deque imported but unused
Remove unused import
(F401)
6-6: collections.OrderedDict imported but unused
Remove unused import
(F401)
7-7: datetime.datetime imported but unused
Remove unused import
(F401)
7-7: datetime.timedelta imported but unused
Remove unused import
(F401)
9-9: json imported but unused
Remove unused import: json
(F401)
220-220: Do not use bare except
(E722)
🔇 Additional comments (5)
main.py (1)
147-153: Ensure intended end-of-algorithm log handling
Separating the logic for live vs. backtest mode with a position store vs. daily log output is clear. Verify that callingprocess_and_output_daily_logs()outside live mode is the intended final step, so logs aren’t missed in any future expansions or edge cases.Tools/Logger.py (4)
11-42: Good encapsulation inLogMessage
The use of hashes for grouping and numeric extraction is well-designed, promoting flexibility and swift grouping in the logger. This class improves readability and modularity.
185-191: Initialize logger storage conditionally
Initializinglogger_storageif it doesn't exist ensures data is consolidated in a single place. This is a good approach, but be mindful of concurrent usage across multiple classes if concurrency is later introduced.
225-275: Efficient daily log processing
This method neatly segregates the logs and clears old entries. Confirm that the logic for skipping processing whencurrent_day == self.last_summary_dayaligns with your daily reset expectations, especially if running across midnight boundary conditions.
276-280: Immediate logging output
Formatting logs immediately for production or live mode can be invaluable for monitoring. The approach is straightforward and consistent with the rest of the logger design.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
==========================================
+ Coverage 71.33% 72.04% +0.71%
==========================================
Files 30 30
Lines 2693 2851 +158
==========================================
+ Hits 1921 2054 +133
- Misses 772 797 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (1)
6-21: Clarify and Standardize Installation Instructions.
The revised setup steps clearly guide users to install PDM and adjust subsequent workflow steps accordingly. Consider ensuring consistent capitalization (e.g., use "QuantConnect" instead of "quantconnect") and punctuation throughout the instructions. Also, verify that the renumbering of steps (from 1 to 8) reads naturally for users..github/workflows/tests.yml (1)
31-32: Use of PDM for Installing Dependencies.
Switching topdm installalong with listing the installed packages viapdm listhelps with debugging. If the debug output isn’t intended for production, consider documenting its purpose or removing it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Pipfile.lockis excluded by!**/*.lockpdm.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/workflows/tests.yml(2 hunks).pdm-python(1 hunks)Pipfile(0 hunks)README.md(1 hunks)pyproject.toml(1 hunks)requirements.txt(0 hunks)run_tests.sh(1 hunks)
💤 Files with no reviewable changes (2)
- requirements.txt
- Pipfile
✅ Files skipped from review due to trivial changes (2)
- .pdm-python
- pyproject.toml
🔇 Additional comments (6)
.github/workflows/tests.yml (3)
24-28: Transition to PDM for Dependency Management.
The addition of the "Install PDM" step (usingpip install pdm) properly replaces the old pipenv setup. This change is clear and necessary for standardizing dependency management.
36-38: Running Tests with PDM.
The updated command (CI=true pdm run ./run_tests.sh) correctly transitions the test execution from pipenv to PDM. Verify that theCIenvironment variable is properly propagated in all CI environments to ensure consistent behavior.
56-59: Updating Artifact Upload Action.
Updating toactions/upload-artifact@v4is a good move to leverage the latest features and improvements. Ensure that any configuration changes required by version 4 are addressed or documented as needed.run_tests.sh (3)
36-40: Migrating Test Execution to PDM Run Commands.
The CI condition now appropriately usespdm run mamba Tests/specs --enable-coverage --format=junit > junit.xml, while the non-CI branch runs without the XML output. This migration from pipenv to PDM appears to be correctly implemented. Confirm that the generated test artifacts (likejunit.xml) are properly integrated into the CI reporting pipeline.
43-45: HTML Coverage Report Generation with PDM.
The command to generate HTML coverage reports usingpdm run coverage htmlalong with the specified include and omit options is correctly configured. Ensure that the--includeand--omitpatterns accurately reflect the desired files and directories.
48-52: Conditional XML Coverage Report Generation in CI.
Generating the XML coverage report only when$CIis set to true is an efficient approach. Verify that the XML output conforms to the requirements of your coverage analysis tools (e.g., Codecov).
Chocksy
force-pushed
the
feat/improved_logger
branch
from
a6c0a1b to
4e95730
Compare
1 participant
Improves Logger. Tries to verify if a set of logs have already been printed and if not then it starts printing them.
Tries to reduce the amount of logs we consume
Summary by CodeRabbit
These updates deliver more structured log insights and smoother end-of-cycle operations for improved system observability.